Skip to content

Feature/flake8#4237

Merged
psychedelicious merged 8 commits intoinvoke-ai:mainfrom
lillekemiker:feature/flake8
Aug 18, 2023
Merged

Feature/flake8#4237
psychedelicious merged 8 commits intoinvoke-ai:mainfrom
lillekemiker:feature/flake8

Conversation

@lillekemiker
Copy link
Contributor

@lillekemiker lillekemiker commented Aug 11, 2023

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because:

Have you updated all relevant documentation?

  • Yes
  • No
  • N/A

Description

As per my suggestions here https://github.com/invoke-ai/InvokeAI/discussions/3721
I am adding flake8 checks to GHA, to pre-commit hooks and also cleaning up flake8 issues.

Related Tickets & Documents

  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

[optional] Are there any post deployment tasks we need to perform?

lstein
lstein approved these changes Aug 14, 2023
View reviewed changes
Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. flake8 seems to have caught lots of problems. Better get this in before more conflicts accrue. I see a bunch already.

@lstein
Copy link
Collaborator

lstein commented Aug 17, 2023

@lillekemiker I'm watching the conflicts build up. Are you planning to re-run flake8 on current main and update the PR?

@lillekemiker
Copy link
Contributor Author

lillekemiker commented Aug 17, 2023

@lillekemiker I'm watching the conflicts build up. Are you planning to re-run flake8 on current main and update the PR?

So flake8 only finds the errors, I have to correct them myself, unfortunately. But I don't mind.
@psychedelicious mentioned a couple of PRs that he wanted merged before this one, so I was mostly waiting for a go-ahead there. I forget which ones they were.
That being said, I can start addressing conflicts now so there are less when we are ready.

@psychedelicious
Copy link
Contributor

psychedelicious commented Aug 18, 2023

@lillekemiker Ah, right - those have merged and the path is clear for this PR :)

@lillekemiker
Copy link
Contributor Author

lillekemiker commented Aug 18, 2023

@psychedelicious @lstein - This should be ready merge as soon as the tests clear (which they already have locally)

psychedelicious
psychedelicious approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@psychedelicious psychedelicious left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'd like to get another set of eyes to glance over the changes before merging. Oh, Lincoln has already reviewed, nvm

psychedelicious
psychedelicious reviewed Aug 18, 2023
View reviewed changes
@psychedelicious psychedelicious merged commit c96ae4c into invoke-ai:main Aug 18, 2023
@lillekemiker lillekemiker deleted the feature/flake8 branch August 18, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lstein lstein lstein approved these changes

@blessedcoolant blessedcoolant Awaiting requested review from blessedcoolant blessedcoolant is a code owner

@ebr ebr Awaiting requested review from ebr

@damian0815 damian0815 Awaiting requested review from damian0815

@GreggHelt2 GreggHelt2 Awaiting requested review from GreggHelt2

@StAlKeR7779 StAlKeR7779 Awaiting requested review from StAlKeR7779

@brandonrising brandonrising Awaiting requested review from brandonrising

@maryhipp maryhipp Awaiting requested review from maryhipp

@Kyle0654 Kyle0654 Awaiting requested review from Kyle0654

+1 more reviewer

@psychedelicious psychedelicious psychedelicious approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lillekemiker @lstein @psychedelicious